Open
Conversation
…t to test thoroughly
…omposer, but that is not yet implemented for HTMLPurifier
Co-authored-by: Raimondas Rimkevičius <github@mekdrop.name>
…unused dev dependencies, and clean up scripts
no license info than the wrong info
the autoloader in the vendor folder.
Contributor
Review Summary by QodoComposer integration with vendor directory security relocation
WalkthroughsDescription• Implements Composer-based autoloading for ImpressCMS libraries, replacing bundled PHP libraries with Composer dependencies • Moves HTMLPurifier, SimplePie, and WideImage libraries to Composer management, removing 300+ bundled library files • Adds moveVendorToTrust() functionality to securely relocate Composer vendor directory from web root to trust path during installation • Integrates new vendor move installation wizard page with comprehensive error handling and user feedback • Updates autoloader chain in icms.php and installation bootstrap to support both trust path and root path vendor locations • Implements Composer-based module and theme discovery via InstalledVersions instead of filesystem scanning • Adds composer.json with PSR-4/PSR-0 autoloading configuration for Icms\* and icms_* namespaces • Standardizes codebase to PSR-12 formatting: array syntax [], double quotes, proper brace placement • Updates SimplePie and HTMLPurifier references to use namespaced class constants from Composer packages • Increments database version to 49 and system version to 2.1.0 beta • Fixes minor bugs: undefined array index in theme blocks, array indentation inconsistencies Diagramflowchart LR
A["Bundled Libraries<br/>htmlpurifier, simplepie,<br/>wideimage"] -->|"Migrate to Composer"| B["composer.json<br/>Dependencies"]
B -->|"Install via Composer"| C["vendor/ Directory<br/>in Web Root"]
C -->|"moveVendorToTrust()"| D["Trust Path<br/>vendor/"]
E["Autoloader Chain"] -->|"Load from"| D
F["Module/Theme Discovery"] -->|"Query via"| B
G["Installation Wizard"] -->|"Execute"| C
G -->|"Trigger"| D
File Changes1. htdocs/libraries/icms/core/Filesystem.php
|
Contributor
Code Review by Qodo
✅ 1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
49
to
56
| $_icms_autoload_from_trustpath = false; | ||
| if (file_exists(ICMS_TRUST_PATH . "/vendor/autoload.php")) { | ||
| $_icms_autoload = ICMS_TRUST_PATH . "/vendor/autoload.php"; | ||
| $_icms_autoload_from_trustpath = true; | ||
| } else { | ||
| $_icms_autoload = ICMS_ROOT_PATH . "/vendor/autoload.php"; | ||
| } | ||
| require_once $_icms_autoload; |
Contributor
There was a problem hiding this comment.
1. $_icms_autoload not verified 📘 Rule violation ⛯ Reliability
require_once $_icms_autoload is executed without verifying that the selected autoload path exists, which can trigger a fatal error when neither trust-path nor web-root vendor/autoload.php is present. This violates the requirement to handle dependency-loading failure points gracefully with actionable context.
Agent Prompt
## Issue description
`htdocs/include/common.php` loads Composer autoloading via `require_once $_icms_autoload` without verifying the resolved path exists, which can cause an unhandled fatal error when `vendor/autoload.php` is missing.
## Issue Context
This is core bootstrap code; failures here should be handled with clear, actionable context rather than an opaque fatal include error.
## Fix Focus Areas
- htdocs/include/common.php[49-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+210
to
+219
| if ($icmsConfigPersona["multi_login"]) { | ||
| if (is_object(icms::$user)) { | ||
| $online_handler = icms::handler('icms_core_Online'); | ||
| $online_handler->write(icms::$user->getVar('uid'), icms::$user->getVar('uname'), time(), 0, $_SERVER['REMOTE_ADDR']); | ||
| $online_handler = icms::handler("icms_core_Online"); | ||
| $online_handler->write( | ||
| icms::$user->getVar("uid"), | ||
| icms::$user->getVar("uname"), | ||
| time(), | ||
| 0, | ||
| $_SERVER["REMOTE_ADDR"], | ||
| ); |
Contributor
There was a problem hiding this comment.
3. multi_login key unguarded 📘 Rule violation ⛯ Reliability
$icmsConfigPersona["multi_login"] and $_SERVER["REMOTE_ADDR"] are accessed without isset()/fallbacks, which can trigger notices or errors in edge cases (e.g., missing config or CLI/non-HTTP contexts). This violates the requirement to check array indices before access and provide fallbacks.
Agent Prompt
## Issue description
The code reads `$icmsConfigPersona["multi_login"]` and `$_SERVER["REMOTE_ADDR"]` without checking they exist, which can cause notices or failures.
## Issue Context
This runs during core bootstrap and should be resilient across environments (HTTP vs CLI) and configuration states.
## Fix Focus Areas
- htdocs/include/common.php[210-219]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+162
to
165
| onclick="location.href='<?php echo $wizard->pageURI("+1"); ?>'" class="next"> | ||
| <?php } ?> | ||
| <?php if ($_POST['mod'] != 1) { ?> | ||
| <?php if ($_POST["mod"] != 1) { ?> | ||
| <img src="img/right-arr.png" alt="<?php echo BUTTON_NEXT; ?>" width="16"/> |
Contributor
There was a problem hiding this comment.
5. $_post[mod] unchecked 📘 Rule violation ⛯ Reliability
The installer template reads $_POST["mod"] directly without checking it exists, and uses a loose comparison (!= 1), which can trigger notices and type-juggling behavior. This violates the requirement to check array indices before access and to use strict comparisons.
Agent Prompt
## Issue description
The installer template reads `$_POST["mod"]` without an existence check and compares loosely.
## Issue Context
Installer pages should not emit notices/warnings due to missing POST fields.
## Fix Focus Areas
- htdocs/install/install_tpl.php[162-165]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+188
to
+194
| $proto = | ||
| isset($_SERVER["HTTPS"]) && $_SERVER["HTTPS"] === "on" | ||
| ? "https" | ||
| : "http"; | ||
| $host = htmlentities($_SERVER["HTTP_HOST"]); | ||
| $server_php_self = htmlentities($_SERVER["PHP_SELF"]); | ||
| $base = substr($server_php_self, 0, strrpos($server_php_self, "/")); |
Contributor
There was a problem hiding this comment.
6. baselocation() unguarded $_server 📘 Rule violation ⛯ Reliability
baseLocation() reads $_SERVER["HTTP_HOST"] and $_SERVER["PHP_SELF"] without checking they exist, which can trigger notices in nonstandard environments or proxied/test contexts. This violates the requirement to check array indices before access and provide fallbacks.
Agent Prompt
## Issue description
`baseLocation()` accesses `$_SERVER` keys without checking they exist.
## Issue Context
While typically present in web requests, these keys can be missing in edge cases; the installer should not emit notices.
## Fix Focus Areas
- htdocs/install/class/IcmsInstallWizard.php[188-194]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| $feed->set_feed_url($rssurl); | ||
| $feed->set_cache_duration(3600); | ||
| $feed->set_autodiscovery_level(SIMPLEPIE_LOCATOR_NONE); | ||
| $feed->set_autodiscovery_level(\Simplepie\Simplepie::LOCATOR_NONE); |
Contributor
There was a problem hiding this comment.
7. Bad simplepie namespace 🐞 Bug ✓ Correctness
Admin RSS now calls \Simplepie\Simplepie::LOCATOR_NONE (wrong casing/namespace), which will trigger a class-not-found fatal when that page renders.
Agent Prompt
### Issue description
`htdocs/admin.php` references `\Simplepie\Simplepie::LOCATOR_NONE` which does not match the actual Composer SimplePie class namespace. This will fail at runtime.
### Issue Context
SimplePie is now provided via Composer and used as `\SimplePie\SimplePie`.
### Fix Focus Areas
- htdocs/admin.php[90-90]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+405
to
+419
| static public function getAvailable(): array | ||
| { | ||
| $package_types = ['impresscms-theme', 'impresscms-module']; | ||
|
|
||
| $dirtyList = $cleanList = array(); | ||
| foreach($package_types as $package_type) { | ||
| $dirtyList = \Composer\InstalledVersions::getInstalledPackagesByType($package_type); | ||
| foreach ($dirtyList as $item) { | ||
|
|
||
| $cleanList[$item]['Name'] = $item; | ||
| $cleanList[$item]['Version'] = InstalledVersions::getVersion($item); | ||
| $cleanList[$item]['PrettyVersion'] = InstalledVersions::getPrettyVersion($item); | ||
| } | ||
| } | ||
| return $cleanList; |
Contributor
There was a problem hiding this comment.
8. Getavailable() api break 🐞 Bug ✓ Correctness
icms_module_Handler::getAvailable() now returns Composer package metadata arrays, but installer/modulesadmin treat it as a list of module dirnames; this can produce warnings and break module install/admin flows when such packages are present.
Agent Prompt
### Issue description
`icms_module_Handler::getAvailable()` changed its return type/shape, but existing consumers still assume the legacy string list of module directory names.
### Issue Context
Installer and modules admin build filesystem paths and compute diffs using the returned values.
### Fix Focus Areas
- htdocs/libraries/icms/module/Handler.php[388-420]
- htdocs/install/page_modulesinstall.php[93-99]
- htdocs/modules/system/admin/modulesadmin/modulesadmin.php[97-103]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Use === to compare $entry against '.' and '..' to avoid type coercion and ensure exact string matches.
Return success from copyRecursive and add a recursive flag to getFileCount. When validating vendor copies, verify autoload.php SHA-1 and compare recursive file counts; remove partial copies and return a descriptive error if verification fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This introduces Composer-based autoloading for libraries. No new functionality, just the base framework to start extricating bundled PHP libraries from the core.
To test : execute a 'composer install' to get the necessary files in the vendor folder. This will have been done already in the release archive.